Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: prep for unified dataset schema #1414

Merged
merged 33 commits into from
Sep 20, 2024

Conversation

nitrosx
Copy link
Contributor

@nitrosx nitrosx commented Sep 5, 2024

Description

This PR rename the current datasets DTOs and schemas and introduce a new unified dataset schema/dto.

Motivation

For historical reason, datasets were implemented with two different schemas depending if they were of type raw or derived.
This design introduced complexity in code and increased maintenance cost.
After 2 years of BE v4.x, we came to the conclusion that such differentiation is not needed and datasets can be described with a single schema and dto.

Fixes:

  • reviewed dataset schema
  • renamed current dto to obsolete

Changes:

  • added new unified dto

Tests included

  • No test changes needed
  • Passing? (Merge will not be approved unless this is checked)

Documentation

  • swagger documentation updated [required]
  • official documentation updated [nice-to-have]

@nitrosx nitrosx changed the title Prep for unified dataset schema FIX: prep for unified dataset schema Sep 5, 2024
@nitrosx nitrosx changed the title FIX: prep for unified dataset schema refactor: prep for unified dataset schema Sep 5, 2024
Copy link
Contributor

@Junjiequan Junjiequan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left few comments

src/datasets/dto/update-dataset.dto.ts Outdated Show resolved Hide resolved
src/datasets/dto/update-dataset.dto.ts Outdated Show resolved Hide resolved
src/datasets/dto/update-dataset.dto.ts Outdated Show resolved Hide resolved
src/datasets/schemas/dataset.schema.ts Outdated Show resolved Hide resolved
src/datasets/dto/output-dataset.dto.ts Outdated Show resolved Hide resolved
src/datasets/dto/output-dataset.dto.ts Outdated Show resolved Hide resolved
@nitrosx nitrosx marked this pull request as draft September 5, 2024 14:26
@sbliven
Copy link
Member

sbliven commented Sep 6, 2024

This is a pretty major breaking change (but a good one). I think we should change the prefix to v4 (#1101)

@sbliven
Copy link
Member

sbliven commented Sep 6, 2024

Also note that the change should be documented clearly in the migration page

@Junjiequan Junjiequan force-pushed the swap-4073-single-dataset-schema branch from 5f84530 to b9cf5b3 Compare September 17, 2024 21:11
@nitrosx nitrosx marked this pull request as ready for review September 19, 2024 09:53
@Junjiequan Junjiequan force-pushed the swap-4073-single-dataset-schema branch from 5399365 to 2778acf Compare September 19, 2024 12:02
@Junjiequan Junjiequan force-pushed the swap-4073-single-dataset-schema branch from 793b167 to ef48549 Compare September 19, 2024 12:18
Copy link
Contributor

@Junjiequan Junjiequan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left few comments

src/datasets/datasets.controller.ts Outdated Show resolved Hide resolved
src/datasets/datasets.controller.ts Show resolved Hide resolved
src/datasets/dto/update-dataset.dto.ts Outdated Show resolved Hide resolved
src/datasets/schemas/dataset.schema.ts Show resolved Hide resolved
@nitrosx nitrosx merged commit 1f8c98d into master Sep 20, 2024
8 checks passed
@nitrosx nitrosx deleted the swap-4073-single-dataset-schema branch September 20, 2024 12:08
@sbliven sbliven mentioned this pull request Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants